Manage resource types via OCI (create/get/list/publish/pull/delete/convert) - #249

@claude review

@claude review

Claude finished @chrisghill's task in 2m 19s —— View job

Claude's Review

Todo list

• Gather context (diff, changed files, repo guidelines)
• Review shared internal/oci package
• Review internal/commands/resourcetype/* (create/get/list/publish/pull/delete/convert)
• Review internal/bundle changes (shared packager wiring)
• Review internal/resourcetype/{build,get}.go (SDK migration)
• Post final review

Overall this is a clean refactor — the internal/oci extraction is a nice reduction in duplication, bundle behavior is preserved (publish/pull now just route through oci.Publisher/oci.Puller with the same media type and .mdignore semantics), and the resource-type command surface is well tested (keep_test.go, convert_test.go, publish_test.go cover the tricky edge cases like path traversal and filename-collision on convert). Nice attention to detail — e.g. TestRunConvertDistinctFilesForDuplicateLabels specifically encodes the old buggy behavior it fixes.

A few things worth a look:

1. Stale golangci-lint exclusion for the deleted internal/api package
.golangci.yaml:419-422 still has a revive/var-naming exclusion scoped to path: "internal/api/", but that package is deleted in this PR. Since exclusions.warn-unused: true is set (.golangci.yaml:408), lint will now flag this rule as unused. Worth deleting that rule block in this PR since it's directly caused by this change.
Fix this →

2. packageKeep case-sensitivity asymmetry (internal/commands/resourcetype/publish.go:53-64)
Top-level allowlisted files (readme.md, icon.png, etc.) are matched case-insensitively via strings.ToLower, but referenced instruction/export paths are matched with an exact-case lookup in the referenced map. This is likely intentional (references should match what's actually in massdriver.yaml), but it means a massdriver.yaml referencing ./Instructions/CLI.md on a case-sensitive filesystem would silently drop the file from the package (caught by validateReferencedFiles only if the exact-case file doesn't exist — but if the file happens to exist with different case, it'd still pass validation yet get skipped by packageKeep, since validateReferencedFiles stats filepath.Join(srcDir, norm) which is case-sensitive on Linux too... actually these two functions use the same normalizeRel/casing, so they should agree in practice). Minor, not blocking — just flagging as something to keep in mind since the two allowlist paths behave differently.

3. resourcetype.Build doesn't validate referenced-file paths itself (internal/resourcetype/build.go:94-124)
Build reads instruction/export files via filepath.Join(baseDir, instruction.Path) with no path-traversal guard. This is safe today because RunPublish always calls validateReferencedFiles (which rejects absolute/escaping paths) before validateSchema → Read → Build (internal/commands/resourcetype/publish.go:130-142). But Build/Read are exported and could be called directly by future callers without that guard. Not a bug in this PR, just worth a comment noting the invariant so it doesn't get silently violated later.

4. Minor: symlink escapes not covered
validateReferencedFiles/normalizeRel guard against ..-escaping and absolute paths, but not symlinks inside the resource-type directory that point outside it. This mirrors the pre-existing bundle .mdignore trust model (not a regression), so not blocking — just noting it's the same trust boundary as before.

Nothing here blocks merging — items 1 is a quick cleanup, 2–4 are minor/defense-in-depth notes rather than bugs found in the new code paths. The SDK migration (internal/resourcetype/get.go), OCI pull/publish flows, and convert round-trip logic all look correct and are well covered by tests.